Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the PR template file from ufs-srweather-app and regional_workflow to the UFS_UTILS repo. #575

Merged
merged 10 commits into from
Sep 14, 2021
Merged

Conversation

JeffBeck-NOAA
Copy link
Collaborator

@JeffBeck-NOAA JeffBeck-NOAA commented Aug 24, 2021

This PR adds the following template file to the repository, which will automatically populate a new PR message when a developer creates a PR in GitHub. The developer then fills in the corresponding information before creating their PR.

  • Use this template to give a detailed message describing the change you want to make to the code.

  • You may delete any sections labeled "optional".

  • If you are unclear on what should be written here, see https://github.com/wrf-model/WRF/wiki/Making-a-good-pull-request-message for some guidance.

  • The title of this pull request should be a brief summary (ideally less than 100 characters) of the changes included in this PR. Please also include the branch to which this PR is being issued.

  • Use the "Preview" tab to see what your PR will look like when you hit "Create pull request"

--- Delete this line and those above before hitting "Create pull request" ---

DESCRIPTION OF CHANGES:

One or more paragraphs describing the problem, solution, and required changes.

TESTS CONDUCTED:

Explicitly state what tests were run on these changes, or if any are still pending (for README or other text-only changes, just put "None required". Make note of the compilers used, the platform/machine, and other relevant details as necessary. For more complicated changes, or those resulting in scientific changes, please be explicit!

Please specify if the relevant consistency tests were run and whether they succeeded. If baselines need to be updated as a result of this PR, please specify the machine(s) and path(s) to the new files.

DEPENDENCIES:

Add any links to pending PRs that are required prior to merging this PR. For example:

NOAA-EMC/UFS_UTILS/pull/<pr_number>

DOCUMENTATION:

If this PR is contributing new capabilities that need to be documented, please also include updates to the RST files in the docs/source directory as supporting material.

ISSUE:

If this PR is resolving or referencing one or more issues, in this repository or elsewhere, list them here. For example, "Fixes issue mentioned in #123" or "Related to bug in https://github.com/NOAA-EMC/other_repository/pull/63"

CONTRIBUTORS (optional):

If others have contributed to this work aside from the PR author, list them here

@JeffBeck-NOAA JeffBeck-NOAA changed the title Add the PR template file from ufs-sreawther-app and regional_workflow to the UFS_UTILS repo. Add the PR template file from ufs-srweather-app and regional_workflow to the UFS_UTILS repo. Aug 24, 2021
@GeorgeGayno-NOAA
Copy link
Collaborator

Under 'tests conducted' we should ask if the relevant consistency tests were run. And if any tests failed.

@GeorgeGayno-NOAA
Copy link
Collaborator

I included this information, with some rewording, under the UFS_UTILS wiki:
https://github.com/NOAA-EMC/UFS_UTILS/wiki/9.-Creating-a-Pull-Request

So instead of pointing to the wrf wiki, you can point to our wiki.

@GeorgeGayno-NOAA
Copy link
Collaborator

Under 'ISSUES', we ask for external issues and PRs. And under 'DEPENDENCIES' we ask for external PRs. Can these be combined, or do I not understand the difference?

@JeffBeck-NOAA
Copy link
Collaborator Author

The dependency section is for PRs that are pending, but are required prior to merging a given PR. They would be listed in this section. The Issues section is only for referencing issues (not PRs) that are resolved by a given PR. I modified the dependency section to be a bit clearer.

@JeffBeck-NOAA
Copy link
Collaborator Author

Under 'tests conducted' we should ask if the relevant consistency tests were run. And if any tests failed.

Added.

@GeorgeGayno-NOAA
Copy link
Collaborator

Since we require unit tests, I suppose we should add under "tests conducted" something like "specify if the changes require any new or updated unit tests"

@edwardhartnett
Copy link
Collaborator

All changes to code require new or changed unit tests.

Asking people if they have run tests is not reliable. People don't run the tests that they are supposed to, that's why automated testing was invented and is in use.

Currently all consistency checks are being run every night by cron, and unit tests are being run by GitHub for every PR. So we should not care what manual testing the contributor has done. So ask long as we stick firmly to the requirement that all new code and code changes must be accompanied by testing, that should work.

@JeffBeck-NOAA
Copy link
Collaborator Author

Updates to the "tests conducted" section were pushed that describe the requirement for contingency tests to be run for each PR. This requirement is important since new PRs/new commits to existing PRs may come in between automated nightly tests and could get merged without being tested. Users should describe any new unit tests or updates to existing ones as well.

@GeorgeGayno-NOAA GeorgeGayno-NOAA merged commit f9b353f into ufs-community:develop Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants